Skip to content

feat(testing): deterministic process-signal lifecycle support in Repl.Testing - #90

Open
carldebilly wants to merge 16 commits into
mainfrom
dev/cdb/issue-82-signal-testing
Open

carldebilly wants to merge 16 commits into
mainfrom
dev/cdb/issue-82-signal-testing

Conversation

@carldebilly

Copy link
Copy Markdown
Member

Closes #82.

PR #80 gave the framework process-wide first/second-signal coordination. Consumers of the shipped Repl.Testing package had no supported way to test it: the behaviour was reachable only through internals, and through a runner internal to this repository's own integration tests. docs/testing-toolkit.md did not mention signals at all.

Two halves, because they prove different things

ReplProcessSignalHarness drives the lifecycle in memory — deterministic, fast, and able to declare any platform's decisions from any host. ReplProcessProbe spawns an application and sends it real signals, for the one guarantee no in-memory test can make: that a process actually terminates, with the code a shell sees.

It is a sibling of ReplTestHost rather than part of it. Sessions are isolated from one another; signal handling is process-global. ReplSessionHandle also serialises one command at a time, which makes a late joiner inexpressible.

Every platform's decisions, from every platform

The signal path had three platform decision points and only two were injectable. The third — the gate on the SIGTERM registration — read the host directly, so "what this platform wires up" could only be asserted on that platform.

It is a wiring decision, not a capability limit: dotnet/runtime's PosixSignalRegistration.Windows.cs maps SIGTERM onto CTRL_SHUTDOWN_EVENT, so .NET accepts the registration on Windows too; the gate exists because the console coordinator already owns Ctrl+C and Ctrl+Break there. It now comes from a policy in force during test isolation.

Because .NET would accept it, a declared platform must never install a live handler in the test runner's process — so that is a separate bit, and the harness never sets it. Measured result: Repl.Tests went from one platform-skipped test to zero. The orphaned-registration test that was excluded on Windows now runs there, and its passing is itself the empirical confirmation of the runtime-source reading above.

What it will not claim

A second signal reports WouldTerminateProcess. That is the framework's decision and nothing more — nothing dies in-process, so the run keeps unwinding and the code after the call keeps executing. docs/testing-toolkit.md carries a table of what each half proves, so "the process actually terminated" is visibly the probe's alone.

Three contract details cost a failing test to discover and are documented so they cost a consumer none: starting a run guarantees a signal will reach it, not that the command is running; signal diagnostics belong to the delivery rather than to a run, because in production they are written from a callback thread with no session; and a late joiner needs the earlier run to still hold its scope, since an epoch resets once its last run drains.

Signals are delivered by the probe on Unix only. Doing it on Windows needs a console control event and console attachment, which would pre-empt the decisions #83 exists to settle; SendSignalAsync refuses there with a message naming the alternative, and everything else works everywhere.

Reviewed locally before pushing

Four reviewers ran on the complete diff. Three findings were right about code and prose I had written, and all three are fixed in e0edd9f:

  • The isolation guarantee was false. UseRealProcessSignalRegistrations claimed to gate whether the harness competed for the runner's own signals; it gated only the SIGTERM registration, while the console cancel-key registration — the one that subscribes the process-wide Console.CancelKeyPress — was unconditional. The option is gone, the harness now never installs an operating-system registration at all, and what genuinely cannot be isolated is documented rather than denied.
  • Disposal could strand ownership for the whole suite. Draining enumerated the run list without the gate that appends to it, and the cleanup had no finally, so one throw left the exclusivity flag set and every later harness refused to start. Reverting the fix fails three tests, including a deterministic one.
  • The stress script covered none of the new classes. Given_ProcessSignalHarness does not contain the substring Given_ProcessSignals. Filter broadened, minimum raised 8 → 32.

Also: a documented example delivered a signal without starting a run and asserted the wrong value; two acceptance criteria were only provable through internals and now have tests at the public surface; ReplSignalRunResult dropped its primary constructor to avoid baking in a Deconstruct-arity break; ReplPlatformProfile's flags are readable but no longer settable.

Verification

  • Release build -warnaserror: 0 warnings.
  • Repl.Tests 787/787 (0 skipped, was 1) · Repl.IntegrationTests 596, 10 skipped (the platform-bound real-signal tests) · Repl.McpTests 225/1 · Repl.SpectreTests 17 · Repl.ProtocolTests 6.
  • eng/ci/process-signal-stress.sh green on the broadened filter.
  • markdownlint-cli2 "docs/**/*.md": 0 issues.

Repl.Testing also stops shipping without XML documentation (first commit, isolated): it carried the test-project override although it is a package on NuGet, so consumers got no IntelliSense. Turning generation on armed CS1591 and surfaced 20 undocumented public members, all now documented.

Repl.Testing carried `GenerateDocumentationFile=false`, the override the five test projects share. It
is not a test project though — it is a package on NuGet, and without a documentation file its
consumers get no IntelliSense on any of its public API.

Turning generation on also arms CS1591, which is an error here, so a new public member can no longer
ship undocumented. That surfaced 20 existing members with no `<summary>`: the types were documented,
their members largely were not. This documents all 20 and leaves the build at 0 warnings.

Separate from the process-signal work in this branch so it can be reviewed on its own.
Ctrl+C and Ctrl+Break have `ConsoleCancelKeyCoordinator.HandleCancelKeyForTesting`. SIGTERM had no
counterpart: `HandleSigTerm` is private and reached only from the `PosixSignalRegistration` callback,
so the shared claim logic was exercised in-process only through the console path, and SIGTERM's own
first/second-signal behaviour was covered only by the out-of-process suite.

`TryClaimSignal`'s first guard compares the delivering registration's captured generation against the
current one. A seam must behave like a freshly installed registration, and reading the counter before
taking the gate would race both `TryInitializeRegistrations`' failure path and the test isolation
scope, which each advance it. So the parameter becomes nullable and the comparison moves inside the
lock that already covers everything else — one signature change, no new locked region, and the two
real call sites keep passing an `int` unchanged.

What this does not cover, stated at the seam rather than implied: `HandleSigTerm`'s own translation of
the decision into `PosixSignalContext.Cancel` needs a real signal context and stays covered only
out-of-process.

Four tests in Given_ProcessSignalCancellationScope, the class the stress script targets, so the
changed locked region runs under its epoch races: the first SIGTERM carries 143; a SIGTERM after
Ctrl+C escalates and leaves the first claim's 130 intact; SIGTERM is inert with no active scope; and a
generation advanced under an active scope still claims, which is the null-generation contract.

Suite: 783 tests, 782 passed, 1 skipped (Windows).
…stalling one

The signal path had three platform decision points and only two were injectable: Ctrl+Break's
`isWindows`, and the bridge-support predicate. The third — the `!OperatingSystem.IsWindows()` gate on
the SIGTERM registration — read the host directly, so "what this platform wires up" could only be
asserted on that platform.

It is a wiring decision, not a capability limit. dotnet/runtime's PosixSignalRegistration.Windows.cs
maps SIGINT/SIGQUIT/SIGTERM/SIGHUP onto console control events, SIGTERM to CTRL_SHUTDOWN_EVENT, so
.NET accepts the registration on Windows too; the gate exists because the console coordinator already
owns Ctrl+C and Ctrl+Break there. The decision therefore comes from a `SignalRegistrationPolicy` in
force during test isolation rather than from the host.

Whether a real registration may be created is a deliberately separate bit on that policy, defaulting
to false. A test declaring a platform it is not running on must not install a live handler in the test
runner's process, and — precisely because .NET would accept it — nothing else would have stopped it.
Suppressing the registration does not suppress delivery: the in-process seams reach the claim logic
either way. The one thing not suppressed is the framework's own process-lifetime
`Console.CancelKeyPress` subscription, which `RegisterStandalone` installs once and never removes by
design; a real Ctrl+C is still evaluated against the real host, never the declared platform.

`TryInitializeRegistrations` split so both halves stay under the 60-line cap.

Two observables for tests: whether the platform in force wants a SIGTERM registration, and whether one
is live. Wanted-but-not-installed is what a platform test looks like, and is the pair that proves no
registration was created on a declared platform's behalf.

`When_RegistrationFailsAfterSigTerm_Then_TheOrphanedRegistrationIsReleased` loses its
`[OSCondition(Exclude, Windows)]`: declaring a non-Windows platform with real registrations allowed
reaches the fail-after-SIGTERM ordering on any host. It also gained an assertion pinning the failure
to the injected fault — without it the test passes vacuously whenever the SIGTERM registration is
what failed, leaving no orphan and never reaching the cleanup it exists for.

Repl.Tests: 787 tests, 787 passed, 0 skipped — the suite now has no platform-skipped test.
Consumers had no supported way to test the signal lifecycle PR #80 introduced. It was reachable only
through internals and through a runner internal to this repo's own integration tests, and
Repl.Testing mentioned signals nowhere. ReplTestHost could not be the answer: sessions are isolated
from one another while signal handling is process-global, and its handle serialises one command at a
time, which makes a late joiner impossible to express.

So ReplProcessSignalHarness is a sibling, not a member. It owns process-signal handling for its
lifetime, runs the application through the only overload that installs the standalone bridge — the
one taking no IServiceProvider, IHost or IReplHost, since every other overload drops the option with
a diagnostic nobody asserts on — and delivers signals the way the operating system would. Every
platform's decisions can be declared, so a Windows wiring decision is assertable from Linux and the
other way round, and no operating-system registration is created on a declared platform's behalf.

SendSignal is synchronous, not async: a signal callback owes the operating system a suppression
decision before it returns, and the framework decides synchronously. An async signature would have
described something the framework does not do.

Only one harness can be alive at a time, enforced rather than documented. Taking ownership tears down
and reinstalls shared registration state, so a second one corrupts the first's isolation instead of
merely racing on the application — the failure has to be loud. The message and the type's own docs
name the per-framework parallelism switches, because the package references no test framework and
cannot apply one itself.

Writing the tests first found three things worth keeping:

- Starting a run guarantees a signal will reach it, not that the command body is executing: the scope
  is installed before arguments are parsed. A test asserting on cleanup must have its command say when
  it is running. Now documented on StartRunAsync rather than left to be discovered.
- Signal diagnostics are written from whichever context delivers the signal, which in production is an
  operating-system callback thread with no session. They belong to the delivery, not to a run, so they
  are captured on the harness and ReplSignalRunResult.DiagnosticText says what it does and does not
  hold.
- A late joiner needs the earlier run to still hold its scope. Once the last run of a claimed epoch
  drains, the epoch resets and the next signal is a first signal again — so the test holds the first
  run inside its cleanup instead of assuming the window stays open.

The tokenizer and the ANSI normalizer moved to a shared internal helper rather than being copied for
the second entry point.

13 tests. Suites: Repl.Tests 787/787, Repl.IntegrationTests 585 with 8 skips (the Linux-only
real-process suite), McpTests 225/1 skip, SpectreTests 17, ProtocolTests 6. Solution at 0 warnings.
The spawn-and-signal machinery already existed, in the wrong place: ShellCompletionTestHostRunner and
Given_ProcessSignals' helpers are internal to this repository's integration tests, so a consumer had
to rediscover PID lifetime, output draining, timeouts and forced cleanup for themselves.

ReplProcessProbe generalises them for an arbitrary executable. Output is drained from the moment the
process starts, so a child that fills its pipe is never blocked by the probe; every wait carries what
was captured into its failure message, because a test that waited on the wrong marker is otherwise
indistinguishable from one whose signal never arrived; and disposal kills the tree, so a failed
assertion cannot leak a blocked process into the rest of the suite. Signalling a process that has
already exited is refused rather than attempted, since a reused id would reach something else.

Signals are sent on Unix only, and refused loudly on Windows with a message naming what to use
instead. Delivering one to another process there needs a console control event and console
attachment, which would pre-empt the Windows decisions issue #83 exists to settle. Everything else —
spawning, waiting on output, exit codes, cleanup — works on every platform, so a cross-platform suite
shares all of it and skips only the delivery.

Ctrl+Break is refused on every platform: it is a Windows console event, and the nearest Unix signal,
SIGQUIT, is one this framework deliberately leaves unclaimed. Mapping it silently would have a test
assert against a path that is not the one it named.

The test host's process-signal scenario now echoes READY to standard output as well as to its marker
file, so a caller can watch the stream. The file stays: it is the only way to observe what happened
during a shutdown the process may not survive long enough to flush, and that distinction is
documented on the probe's Output property rather than left as a trap.

6 tests. Two are Unix-only by nature — real delivery is the one guarantee no declared platform can
substitute for — and the Windows-refusal test is the mirror image, so the CI matrix covers both
sides. Suites: Repl.Tests 787/787, Repl.IntegrationTests 591 with 10 skips, McpTests 225/1,
SpectreTests 17, ProtocolTests 6.
It drives the same process-global coordinator and cancel-key state as every other class carrying
[DoNotParallelize], but carried none itself. That was safe only by accident: because every other
class touching that state is tagged, MSTest scheduled them in its serial pass and left this one alone
in the parallel one. The moment a second untagged class touches the same statics, both start racing —
and the process-signal harness this branch adds is exactly the kind of thing that would.
docs/testing-toolkit.md had no mention of signals at all, so the two halves now have a section: the
in-memory harness for every decision the framework makes, and the spawned-process probe for the
guarantees only a real process can give.

Most of it is the boundary rather than the API. A second signal makes the harness report
WouldTerminateProcess, which is the framework's decision and nothing more — nothing dies in-process,
so the run keeps unwinding and the code after the call keeps executing. StartRunAsync guarantees a
signal will reach the run, not that the command is running, and a test asserting on cleanup has to
have its command say when it started. Signal diagnostics belong to the delivery rather than to a run,
because in production they are written from an operating-system callback thread with no session. A
late joiner needs the earlier run to still hold its scope, since an epoch resets once its last run
drains. Each of those cost a failing test to discover; none of them should cost a consumer one.

The parallelism guidance is per framework — MSTest, xUnit and NUnit differ in whether they parallelise
by default — because the package references none of them and cannot apply the setting itself. A table
says what each half proves, so "the process actually terminated" is visibly the probe's alone.

Cross-references from the configuration reference and best practices, both of which describe signal
ownership without previously saying how to assert it, plus the package readme. Verified that
docs/testing-toolkit.md is what publishes to /cookbook/testing/, so the anchor resolves.
Four reviewers ran before pushing. Two verdicts were approve-with-changes, skeptic's was fix-first, and
three of its findings were right about code I had written and prose I had asserted.

**The isolation claim was false, and so was its documentation.** `UseRealProcessSignalRegistrations`
said that leaving it off kept the harness from competing for the test runner's own signals. Only the
SIGTERM registration was ever gated by it: `ConsoleCancelKeyCoordinator.RegisterStandalone` is called
unconditionally, and that is what subscribes the process-wide `Console.CancelKeyPress`. A real Ctrl+C
during a harness run was claimed cooperatively whatever the flag said.

The flag is gone. It had no observable effect through the public API either — the thing it existed to
let you assert lives on an internal member, and exposing one now would pre-empt the observability API
issue #84 is for. The harness now never installs an operating-system registration, which is a stronger
promise than the one the flag was guarding. What cannot be isolated is documented instead of denied:
starting a run registers a console cancel-key handler, because arbitrating Ctrl+C between an
interactive session and a standalone run is part of what these tests exercise, so a real Ctrl+C aimed
at the runner is claimed by the run under test and the first press does not stop it.

**Disposal could strand process-signal ownership for the rest of the suite.** `DrainRunsAsync`
enumerated the run list without the gate that `StartRunAsync` takes to append to it, and the cleanup
after it had no `finally`. A throw from the drain left the exclusivity flag set and the coordinator
isolated, so every later harness in the process refused to start — one failed disposal cascading into
an entire suite. Disposal now takes the start gate and releases ownership unconditionally.

Proven rather than asserted: reverting to the unguarded form fails three tests, including the new
deterministic one that drains a run which timed out.

**The stress script never covered any of this.** `Given_ProcessSignalHarness` does not contain the
substring `Given_ProcessSignals` — there is no "s" after "Signal" — so the integration filter matched
none of the new classes. The unit half did match its own filter, so the coordinator change was under
stress; the harness and probe were not. Filter broadened, minimum raised from 8 to 32 so a future
filter that silently stops matching fails loudly.

**A documented example could not work.** The declared-platform snippet delivered a signal without
starting a run, so it asserted `CancellationRequested` where the code returns `NotHandled`. A reader
copying it failed on their first try.

Also from the panel: two acceptance criteria were only provable through internals a package consumer
cannot reach, so a failing cancellation callback and a run whose own outcome outranks a claimed signal
now have tests at the public surface; `ReplSignalRunResult` drops its primary constructor, since a
positional record bakes in a Deconstruct-arity break for any member added later; `ReplPlatformProfile`
keeps its flags readable but no longer settable, so combinations no device has cannot be built;
`ScopeRegisteredCallbackForTesting` folds into the isolation scope rather than sitting beside it as a
fifth, unowned static; and `ReplSignalRun.Completion` documents the local-variable pattern that avoids
VSTHRD003.

Release build 0 warnings. Repl.Tests 787/787, IntegrationTests 596 with 10 skips, McpTests 225/1,
Spectre 17, Protocol 6. Stress script green on the broadened filter.
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 12, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-15T00:31:43.774816Z d02df18 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e0edd9ff0d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/Repl.Testing/ReplProcessSignalHarness.cs Outdated
Comment thread src/Repl.Testing/ReplProcessSignalHarness.cs
Comment thread src/Repl.Testing/ReplProcessProbe.cs
Comment thread src/Repl.Testing/ReplProcessSignalHarness.cs Outdated
Comment thread src/Repl.Testing/ReplProcessSignalHarness.cs
Comment thread src/Repl.Testing/ReplProcessProbe.cs
Comment thread src/Repl.Testing/ReplProcessSignalHarness.cs
Comment thread src/Repl.Testing/ReplTestText.cs
Comment thread src/Repl.Testing/CommandExecution.cs Outdated
Comment thread src/Repl.ShellCompletionTestHost/Program.cs Outdated
Ten threads, all of them right.

**Dogfooding, from the owner.** The test host wrote its readiness marker with `Console.WriteLine`.
It goes through `IReplIoContext.Output` now, like any other command in this repository.

**A declared platform could claim a signal it would never receive.** `SendSignal(Terminate)` called
the claim logic directly, bypassing the wiring decision the declared platform makes. On a declared
Windows profile the framework installs no SIGTERM registration, and on an unsupported one it installs
nothing at all, so the harness reported a cancellation that could not happen on the platform the test
named — the exact false positive the platform-declaration feature exists to prevent.

Gated on whether SIGTERM is actually wired for the platform in force. Two of this branch's own tests
failed immediately on that change, because both asserted a SIGTERM claim under the default profile,
which on a Windows host is Windows. They were asserting the false positive. They declare Unix now, so
they say what they mean and pass identically on every host.

**A swallowed timeout was reported as success.** The timeout only surfaced through an exception
filter, so an application that maps cancellation to an exit code — or a command that catches it —
returned normally and the run looked like it had succeeded, for a signal that never arrived. The
timeout is now checked after a normal return too, the way `ReplSessionHandle` already does.

**A start queued behind disposal could launch into a torn-down harness.** `StartRunAsync` checked
disposal before waiting on the start gate, so a start that queued while disposal held the gate would
resume afterwards and run outside the isolation that had just been dismantled. Re-checked after
acquiring it.

**Isolating while someone else's run is in flight corrupts that run.** Taking ownership tears down
registrations without touching the scopes using them, so an unrelated automatic run would keep its
place in the epoch and be cancelled by this harness's first signal. Creation is refused while any
scope is active, with a message saying why.

Also: the probe could reject a child that had written its marker but not yet flushed it through the
asynchronous capture, and could throw during disposal when the child exited between the check and the
kill; a failed run left its explicitly named session in the process-wide dictionary; the tokenizer was
documented as shell-like when it splits on whitespace and groups with double quotes and nothing more;
and `TimelineEvents` promised an ordering between output and interactions that `BuildTimeline` does
not produce — output is one aggregate event, not interleaved.

Three new guards: SIGTERM honours the declared wiring, a swallowed timeout still fails, and a harness
is refused while an unrelated run holds a scope.

Release build 0 warnings. Repl.Tests 787/787, IntegrationTests 600 with 10 skips, McpTests 225/1,
Spectre 17, Protocol 6.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 79a0f318de

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/Repl.Testing/ReplProcessSignalHarness.cs Outdated
Comment thread src/Repl.Testing/ReplProcessProbe.cs
Comment thread src/Repl.Testing/ReplProcessSignalHarness.cs
Comment thread src/Repl.Testing/ReplProcessProbe.cs
Comment thread src/Repl.Testing/ReplProcessSignalHarness.cs
Five threads, all correct, three of them P1.

**The harness ran every command as a hosted session.** `ReplSessionIO.SetSession` defaults
`isHostedSession` to true, and `CoreReplApp` turns that into `ReplRuntimeChannel.Session`. So a harness
documented as modelling the process-owning standalone invocation was running the application through
the hosted lens: commands gated to the CLI channel invisible, `IReplIoContext.IsHostedSession` reading
true, output policy on the hosted surface. A fidelity bug in the one thing this PR exists to provide.
Both capture sessions now say `isHostedSession: false`.

**The timeout could not fire for a command that ignores its token.** `CancelAfter` only requests
cancellation. The new test hung for 45 seconds and was killed — not a failing assertion, a stuck
suite, which is precisely what the timeout exists to prevent. It is now measured against the clock:
the run's completion faults whatever the run is doing, and draining on disposal is bounded the same
way. Such a run cannot be killed, so it is abandoned — and because it still holds a place in the
process-wide epoch, disposal reports it rather than letting the next test inherit it. Draining allows
twice the run timeout, since it can begin before a run's own timeout has elapsed and the run still has
to unwind after it fires; a run that faulted on its own timeout is a finished run, not an abandoned
one.

**Harness exclusivity was a snapshot, not a claim.** Checking `ActiveScopes` once left a gap: a run
registering afterwards joined the harness's isolated epoch, fired its shared readiness callback —
releasing `StartRunAsync` before the harness's own run had registered — and was then cancelled by the
next `SendSignal`. The coordinator now takes an ownership claim atomically with that check and holds
it until released; while held, `Register` refuses any scope the owner did not launch, and the
readiness callback fires only for owned ones. The harness marks its runs with an `AsyncLocal` before
launching them, so the marker flows into the context where the scope is constructed. A concurrent
unrelated run now fails loudly, which is the right outcome: it is concurrent with a signal test, which
the documentation already forbids. `ActiveScopeCountForTesting` is gone, superseded.

Probe: a marker written during the final poll was rejected even though the failure message contained
it — one last check before giving up. And the process-tree cleanup guarantee was overstated: the tree
is only reachable while its root is, so a child that spawns something long-lived and exits leaves it
running. Holding descendants beyond the parent needs a job object or process group, which is
platform-specific work tracked in #91; the documented guarantee now says what the code does.

The stress filter's minimum rises 32 → 39 to match the new tests.

Release build 0 warnings. Repl.Tests 787/787, IntegrationTests 603 with 10 skips, McpTests 225/1,
Spectre 17, Protocol 6. Stress script green.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e5c8f2f292

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/Repl.Defaults/ProcessSignalCoordinator.cs Outdated
Comment thread src/Repl.Defaults/ProcessSignalCoordinator.cs Outdated
Comment thread src/Repl.Testing/ReplProcessSignalHarness.cs Outdated
Comment thread src/Repl.Defaults/ProcessSignalCoordinator.cs
Comment thread src/Repl.Testing/ReplProcessProbe.cs
Comment thread src/Repl.Testing/ReplProcessSignalHarness.cs Outdated
Comment thread src/Repl.Testing/ReplProcessProbe.cs Outdated
Seven threads, all correct, three P1. Two of them are real flake sources in the ownership work added
last round.

**An empty scope set is not an idle coordinator.** `UnregisterAsync` removes a scope before its
signal-triggered callbacks have drained and keeps the claimed signal alive until they have, so
claiming ownership in that window handed the next harness an epoch that was still claimed — its first
run would be cancelled on registration, with no signal ever sent. The claim now also requires the
pending drain and the claimed signal to have cleared.

**The owned-run marker belonged to nobody in particular.** It was a bare flag, and a flag flows into
anything a handler spawned: a background task outliving its harness would still read as owned and
could join whichever harness owned the coordinator next, even releasing its readiness wait. The marker
now carries the claim itself and is compared by reference, so a stale context is refused.

**The registration wait could hang despite the timeout.** The run's own timeout starts inside the run,
after the application factory returns, so a factory that blocks left `StartRunAsync` waiting on two
tasks that would never complete — and the cancellation token documented as cancelling that wait did
nothing. It is now bounded by both.

**A TimeoutException from the application was relabelled as the harness's.** `WaitAsync` propagates
the run's own exception, and a blanket catch turned a slow provider build into "the run exceeded its
timeout", sending the test after a deadline that had not elapsed. Only the wait's own timeout is
converted now, which is the same distinction the drain already makes.

Probe: a `kill` that never returns left a stray sender process and threw a bare timeout without the
captured output every other wait on that type promises — it is terminated and described now. And
redirected output is read a line at a time, so a marker written without a trailing newline is not seen
until the stream closes; that is now the documented contract rather than a silent trap.

One doc correction of my own making: I wrote that a declared platform never changes how a real signal
is treated. It does, in one case — a profile with no signal bridge installs no console cancel-key
handler either, so a real Ctrl+C during such a run takes its normal course instead of being claimed.
Said plainly in all three places that made the claim.

Two guards added for the coordinator findings; both fail against the previous code.

Release build 0 warnings. Repl.Tests 789/789, IntegrationTests 603 with 10 skips, McpTests 225/1,
Spectre 17, Protocol 6. Stress script green.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4b02768dc6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

return pending;
}

public void Signal() => Volatile.Read(ref _pending)?.TrySetResult();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Tie readiness notifications to the started run

Fresh evidence beyond the existing ownership-marker concern is a registration wait that times out or is canceled while _appFactory() remains blocked: its task continues and can register after a subsequent StartRunAsync has replaced _pending via Arm(). The delayed first run then calls this Signal() and releases the second start before the second run has joined the epoch, so an immediate signal can cancel the first run while missing the run just returned to the caller. Associate each registration notification with its specific start, or prevent another start from arming until the previous startup task has settled.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. Fixed in 8ba874f.

A single _pending slot meant the signal identified "a start is waiting", not "this start is waiting". A run whose registration wait had already given up — because its application factory was still blocked — could, when it finally registered, release a later start, reporting a run as having joined the epoch when it had not.

ScopeRegistrationSignal is a queue now, released oldest-first. Registrations arrive in start order because starting is serialised, so oldest-first pairs each registration with the start that caused it. And a start that gives up on its wait withdraws its own reservation (Abandon), so the late registration cannot be paired with somebody else's instead.

Found again independently by six lenses of a local review panel run on this head, which also turned up a related defect I had missed: a claim could outlive the owner that made it, so an unrelated run joined an epoch nobody was draining and was cancelled by a signal nobody sent — reported as Interrupted with no diagnostic naming a signal. A claim now records its owner, and one whose owner has been released is discarded loudly rather than inherited. Guarded by When_AClaimOutlivesItsOwner_Then_TheNextRunDoesNotInheritIt.

Comment thread src/Repl.Testing/ReplProcessProbe.cs
Comment thread src/Repl.Testing/ReplProcessSignalHarness.cs Outdated
Comment thread src/Repl.Testing/ReplProcessProbe.cs
Eight lenses run locally on the current head. Six independently confirmed the four findings still open
from the last round; four new defects surfaced, three of which come from my own fixes in earlier
rounds rather than from the original design.

**A claim can outlive the owner that made it.** Disposal releases ownership before reporting a run it
could not stop — deliberately, because holding it until the scopes drain is what stranded the whole
process two rounds ago. But the abandoned scope stays in the epoch with its signal claimed, and
`Register` gated only on there being an owner, not on the epoch being clean. The next ordinary run
joined a claim nobody was draining and was cancelled by a signal nobody sent, reported as `Interrupted`
with no diagnostic naming one — from the caller's side, indistinguishable from a bug in their own
application. A claim now records its owner; one whose owner has been released is discarded, loudly,
instead of inherited.

**The readiness signal belonged to no particular start.** A single slot meant a run whose registration
wait had already given up could, when its blocked application factory finally returned, release a
*later* start — reporting a run as having joined the epoch when it had not. It is a queue now, released
oldest-first, and a start that gives up withdraws its own reservation so the late registration cannot
be paired with somebody else's.

**The task handed to callers was never observed.** `_runs` retained the raw run; the wall-clock wrapper
returned as `Completion` was retained by nothing. One of this suite's own tests deliberately never
awaits it, so a faulted task was being left for `TaskScheduler.UnobservedTaskException` on every run —
not a hypothetical caller's mistake, ours. Both are retained now: the raw one still distinguishes an
abandoned run from a finished one, and the wrapper is observed on disposal.

**PID identity, twice.** Signalling checks that the child is alive and then shells out to `kill`, and
the two cannot be made atomic without a handle the operating system keeps alive; walking a process tree
on Unix matches by id with no start-time check either. Neither has a cheap fix, so both now say so
where a consumer meets them, as the descendant-cleanup limit already did. Cancelling a signal now kills
the `kill` helper, which only a timeout did before. And `kill` is resolved from `/bin` or `/usr/bin`
rather than by bare name: started without a shell it went through PATH, so a CI step prepending a
directory decided which binary received the signal — a trust decision this package should not make for
the caller.

Documentation: `TimelineEvents` contradicted itself in consecutive sentences on an already-shipped
member, saying output is captured "at the end" where the summary and the code both put it first.
`DisposeAsync` throws and had no `<exception>` tag. Three rationales stated twice across public docs
are now stated once and cross-referenced. `ReplPlatformProfile` gets a private constructor, so the
named profiles really are the only ones. The stress script's floor says it is a floor and must be
raised by hand.

`ReplTestTimeout` collects the wall-clock timeout the session handle and the harness had each grown
their own copy of — the same reasoning that extracted `ReplTestText` earlier in this branch.

One suggestion tested and not taken: replacing the tests' `VSTHRD003` suppressions with a bare
task-returning lambda does not avoid the analyzer, which flags returning a foreign task as well as
awaiting one. The suppression stays, with the reason recorded next to it.

Release build 0 warnings. Repl.Tests 790/790, IntegrationTests 603 with 10 skips, McpTests 225/1,
Spectre 17, Protocol 6. Stress script green.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8ba874f769

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/Repl.Testing/ReplProcessSignalHarness.cs Outdated
Comment thread src/Repl.Defaults/ProcessSignalCoordinator.cs Outdated
Comment thread src/Repl.Defaults/ProcessSignalCoordinator.cs Outdated
Three findings, all correct, and the second is an invariant I broke myself in the previous commit.

**A queue was still the wrong shape.** Making the readiness signal a queue fixed a slot being
overwritten, but not this: a start that gives up withdraws its own wait, yet its launch keeps running
and may register long afterwards. Oldest-first then hands that late registration to whichever start is
waiting by then, returning a run to the caller before it joined the epoch — so the next signal cancels
the abandoned run and misses the one just handed over.

Order was never something to rely on. The registration callback now carries a per-run token, taken from
the owner marker the harness sets before launching, and the harness keys its outstanding waits by it. A
late registration whose start gave up finds nothing under its token and is discarded instead of being
given to someone else. Guarded by a test that blocks two application factories to build the window; it
fails against order-based pairing.

**The orphan-claim diagnostic was written under the gate.** This class promises no consumer callback
runs while the lock is held, and `ReplSessionIO.Error` is caller-supplied: a writer that blocked there
would stop a concurrent signal callback from reaching its suppression decision. The registration-failure
path already captures and writes afterwards; this one now does the same, and the discard moved into its
own method rather than growing `Register` past the length cap.

**A scope the harness never started keeps the epoch occupied.** A command can start its own automatic
run; the owner marker flows into it, so the coordinator admits its scope — but nothing puts it in the
harness's list, so disposal can return cleanly while it is still active, and every later harness is
refused with nothing saying why. Disposal now reads the scope count before releasing ownership and
reports what outlived it. It cannot drain what it did not start; making the failure legible where it
happens is the part that was missing.

Release build 0 warnings. Repl.Tests 790/790, IntegrationTests 604 with 10 skips, McpTests 225/1,
Spectre 17, Protocol 6. Stress script green.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 690e53939d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/Repl.Testing/ReplProcessSignalHarness.cs Outdated
Comment thread docs/testing-toolkit.md Outdated
**A signal could be reported as handled while reaching somebody else.** Console cancel-key selection is
exclusive — `CaptureSelectionUnsafe` dispatches to the interactive handlers *instead of* the standalone
ones when any exist — so a harness created while an interactive session owned the keys would send
Interrupt into that session, cancel its command, and report `CancellationRequested` while the run under
test was never touched and went on to time out.

A green test asserting a cancellation that never happened is the worst failure this toolkit can have,
so it is refused at both points it can arise: creation, and delivery, since an interactive session can
be registered after a harness exists. Terminate is unaffected and says why — SIGTERM does not
participate in the interactive console-key priority rule.

**The guide still promised what the code no longer does.** `docs/testing-toolkit.md` said disposal kills
the process tree so a failed assertion cannot leak a running process, while the probe's own API
documentation had already been narrowed to say a descendant survives when its parent exits first. The
guide now carries the same limit.

Release build 0 warnings. Repl.Tests 790/790, IntegrationTests 605 with 10 skips, McpTests 225/1,
Spectre 17, Protocol 6.
The guard added in e740b62 only passed on Windows. It asserted that Terminate comes back NotHandled
while an interactive session holds the console keys — true under a declared Windows profile, where no
SIGTERM registration is wired, and false everywhere else: the default profile is the host, so on Unix
SIGTERM is declared and the delivery claims. Five CI jobs failed, all of them Unix-side.

That is precisely the failure this pull request exists to make impossible, written into one of its own
tests. The test declares Unix now, and asserts what it meant to: an interactive owner stands between
Interrupt and the run, and does not stand between SIGTERM and the run, because SIGTERM never goes
through console-key arbitration. Both halves now mean the same thing on every host — the claim
released by SIGTERM is also what lets the run finish, so the outcome is asserted rather than assumed.

Reproduced locally before fixing by declaring Unix on a Windows host, which failed with the same
message CI reported, character for character.

Release build 0 warnings. Repl.Tests 790/790, IntegrationTests 605 with 10 skips, McpTests 225/1,
Spectre 17, Protocol 6. Stress script green.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2cadcf8e63

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +271 to +272
if (signal is not ReplProcessSignal.Terminate
&& ConsoleCancelKeyCoordinator.HasInteractiveHandlersForTesting)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Make the interactive-owner check atomic with dispatch

When an interactive handler registers after this snapshot but before HandleCancelKeyForTesting captures/revalidates its selection, the coordinator switches to the interactive handlers, yet SendSignal reports their result as the harness delivery; an Interrupt can therefore return CancellationRequested while cancelling an unrelated interactive command and leaving the harness run untouched. Fresh evidence after the earlier ownership finding is that the revised guard and dispatch still acquire the console-coordinator lock in separate operations; make the ownership rejection part of the same selection operation used for synthetic dispatch.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. Fixed in d02df18.

Checking HasInteractiveHandlersForTesting and then dispatching took the console coordinator's lock twice, so a handler registering in between still took the delivery — and the harness still reported it as its own. Guarding a race with a snapshot is the same mistake I made two rounds ago on the ownership claim, in the same shape.

The refusal now happens inside ConsoleCancelKeyCoordinator, decided from the same revalidated selection the dispatch would have used, and no handler runs when it refuses. That is the part that matters: reporting an interactive session's result as the harness's own is a test passing on a cancellation that never reached the run it names.

It is raised rather than returned as NotHandled, because the delivery did not go unclaimed — it could not be made, and a test told "nothing happened" would go looking in the wrong place.

One thing I have not done: test the race itself. The existing guard covers the steady state, and the fix is structural — one lock acquisition where there were two — rather than something a test can pin without controlling the interleaving. Saying so rather than implying coverage I do not have.

Comment thread src/Repl.Testing/ReplProcessProbe.cs Outdated
// Exiting does not mean the capture is complete: the last line may still be sitting in an
// asynchronous callback. This overload waits for those handlers to finish, so the recheck
// below sees everything the child actually wrote before the wait is called a failure.
_process.WaitForExit();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep exited-process output draining within the timeout

When the root process exits after spawning a long-lived descendant that inherited its redirected stdout or stderr handle, this parameterless WaitForExit waits indefinitely for the asynchronous readers to observe EOF, which cannot happen until that descendant exits. WaitForOutputAsync therefore never reaches its configured deadline or returns the promised failure. Fresh evidence beyond the documented descendant-cleanup limitation is that this synchronous drain itself is unbounded; drain under the remaining timeout or stop waiting for EOF once the deadline expires.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, and this was the more serious of the two. Fixed in d02df18.

Process.WaitForExit() without a token drains by waiting for end-of-stream on the redirected handles, and a descendant that inherited them holds the stream open until it exits. So the drain outlives the process it was draining, and every deadline above it stops meaning anything — inside the one method whose entire job is to honour one. I added that call two rounds ago to fix a marker being rejected before it had flushed, and introduced this doing it.

The same call was in WaitForExitAsync too, right after its bounded wait, with the same hazard. You named one; both are gone.

They are replaced by a bounded settle: poll the capture until nothing new has arrived for two consecutive intervals, or until a grace clamped to the caller's own deadline. That gives up the certainty of end-of-stream in exchange for terminating, which is the trade a timeout exists to make — and the reasoning lives at the helper, so the next person to touch it knows why it is not simply calling the blocking overload.

…atch

**A drain could outlive the process it was draining.** `Process.WaitForExit()` without a token drains
by waiting for end-of-stream on the redirected handles, and a descendant that inherited them holds the
stream open until it exits — so the wait never returns, in the one method whose entire job is to honour
a deadline. The finding named the drain in `WaitForOutputAsync`; the same call sat in `WaitForExitAsync`
with the same hazard, so both are gone.

They are replaced by a bounded settle: poll the capture until nothing new has arrived for two
consecutive intervals, or until a grace that is itself clamped to the caller's deadline. That trades
the certainty of end-of-stream for terminating, which is the trade a timeout exists to make, and the
reasoning sits at the helper rather than at either call site.

**The interactive-owner refusal is now part of the dispatch.** Checking `HasInteractiveHandlersForTesting`
and then dispatching took the console coordinator's lock twice, so a handler registering in between
still took the delivery and the harness still reported it as its own. The refusal is decided from the
same revalidated selection the dispatch would have used, inside `ConsoleCancelKeyCoordinator`, and no
handler runs when it refuses — which was the point, since reporting an interactive session's result as
the harness's own is a test passing on a cancellation that never reached the run it names.

Refusal is raised rather than returned as NotHandled: the delivery did not go unclaimed, it could not
be made, and a test told "nothing happened" would look in the wrong place.

Not separately tested: the race itself. The existing guard covers the steady state, and the fix is
structural — one lock acquisition instead of two — rather than something a test can pin without
controlling the interleaving.

Release build 0 warnings. Repl.Tests 790/790, IntegrationTests 605 with 10 skips, McpTests 225/1,
Spectre 17, Protocol 6. Stress script green.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d02df188ec

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +371 to +375
while (Clock.GetUtcNow() < limit && settled < 2)
{
await Task.Delay(TimeSpan.FromMilliseconds(25), Clock, cancellationToken).ConfigureAwait(false);
var current = _capture.Read().Length;
settled = current == previous ? settled + 1 : 0;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Wait longer than two quiet polls before declaring output drained

In the revised bounded drain, an exited child whose asynchronous DataReceived callbacks are delayed by thread-pool contention can have no capture growth during these first two polls, causing the method to return after roughly 50 ms even though the 500 ms grace and caller deadline have not elapsed. WaitForOutputAsync can then reject a marker the child already wrote, and WaitForExitAsync can return before its final output is observable; wait for reader completion subject to the deadline, or otherwise keep polling for the full grace period rather than treating brief inactivity as completion.

Useful? React with 👍 / 👎.

Comment on lines +519 to +521
if (registered.IsCompleted)
{
return;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject a run that finishes before StartRunAsync returns

When a short-lived command unregisters its signal scope after setting registered but before this continuation resumes, this branch treats the historical registration as sufficient and returns a ReplSignalRun even though no active scope remains. An immediate SendSignal then returns NotHandled, contradicting the public guarantee that a signal delivered after StartRunAsync returns reaches the run; check that completion has not already won before returning, or narrow the contract so callers do not rely on that guarantee.

Useful? React with 👍 / 👎.

Comment on lines +13 to +14
internal static bool IsEnabled(TimeSpan timeout) =>
timeout > TimeSpan.Zero && timeout != Timeout.InfiniteTimeSpan;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Do not silently disable a zero run timeout

When a caller configures RunTimeout = TimeSpan.Zero, this predicate classifies it as no timeout, so a command waiting indefinitely makes both Completion and harness disposal hang. The new option explicitly documents Timeout.InfiniteTimeSpan as the way to disable the safeguard, while zero is a valid immediate timeout for the underlying cancellation and wait APIs; either honor zero as an immediate deadline or reject it instead of silently removing the bound.

Useful? React with 👍 / 👎.

// a declared platform's wiring becomes assertable from any platform. Whether a real
// registration may be created is a separate bit, because a declared platform must never
// install one in the test runner's own process.
s_sigTermRegistrationDeclared = !IsWindowsForRegistration();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Clear SIGTERM eligibility when bridge initialization fails

Fresh evidence after the declared-platform delivery guard is that this flag is set before the console-key registration succeeds. If RegisterStandalone subsequently throws on a host that cannot install the console subscription, the catch path diagnoses the entire automatic bridge as unavailable and disposes any partial registration but leaves this flag true; ReplProcessSignalHarness.SendSignal(Terminate) then bypasses its new guard and reports CancellationRequested even though the failed bridge could never receive SIGTERM. Reset the declaration on failed initialization or gate synthetic delivery on successful bridge initialization as well.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add deterministic process-signal lifecycle support to Repl.Testing

1 participant